fix: stabilize failing ElevenLabs, HuggingFace, and Sarvam tests - #636
fix: stabilize failing ElevenLabs, HuggingFace, and Sarvam tests#636DaemonLoki wants to merge 4 commits into
Conversation
Co-authored-by: Cursor <cursoragent@cursor.com>
📝 WalkthroughWalkthroughUpdated Sarvam support to remove Merge Risk: 🟡 Moderate · up to The PR updates provider-specific tests and model references, but its interruption coverage still depends on mocked behavior and permits more generated output than the intended cancellation contract. Merge should wait for these test corrections or explicit owner acceptance. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (2)
plugins/huggingface/tests/test_transformers_vlm.py (2)
148-150: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReplace the mock-based generation setup.
Line 148 configures
MagicMock.generatewithside_effect. Use a concrete test-only model implementation and constructVLMResourceswith it. This keeps the interruption test deterministic without mocks.As per coding guidelines,
**/*test*.pysays “Never mock in tests; use pytest for testing.”Source: Coding guidelines
348-348: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse direct attribute assignment.
Line 348 uses
setattr. Assignresources.model.generatedirectly.Proposed fix
- setattr(resources.model, "generate", signaling_generate) + resources.model.generate = signaling_generateAs per coding guidelines,
**/*.pysays “Avoidgetattr,hasattr,delattr,setattr; prefer normal attribute access.”Source: Coding guidelines
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: c7501ab9-205a-42f9-ba77-1036ffa7e8bf
📒 Files selected for processing (6)
CHANGELOG.mdplugins/elevenlabs/tests/test_elevenlabs_stt.pyplugins/huggingface/tests/test_transformers_vlm.pyplugins/sarvam/README.mdplugins/sarvam/tests/test_sarvam_llm.pyplugins/sarvam/vision_agents/plugins/sarvam/llm.py
Included review availability: Your plan provides up to 8 included reviews per hour; 7 remain after this review.
Co-authored-by: Cursor <cursoragent@cursor.com>
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
plugins/huggingface/tests/test_transformers_vlm.py (2)
127-160: 📐 Maintainability & Code Quality | 🟠 Major | 🏗️ Heavy liftDo not add
MagicMock-based tests here.The new interruption test uses mocked model and processor behavior. Replace these mocks with concrete test implementations, or keep interruption coverage in the real integration test.
As per coding guidelines, "
**/*test*.py: Never mock in tests; use pytest for testing."Source: Coding guidelines
362-367: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winAssert the one-token interruption contract.
generated_token_counts[0] < 20allows 19 tokens after interruption. The upstream_CancelStoppingCriteriacontract inplugins/huggingface/vision_agents/plugins/huggingface/transformers_llm.py:160-175requires generation to stop within one token. Assertgenerated_token_counts[0] <= 1so this test detects delayed cancellation.Proposed assertion
- assert generated_token_counts[0] < 20 + assert generated_token_counts[0] <= 1
🧹 Nitpick comments (1)
plugins/huggingface/tests/test_transformers_vlm.py (1)
348-348: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse normal attribute assignment instead of
setattr.Replace this call with
resources.model.generate = signaling_generate.As per coding guidelines, “Avoid
getattr,hasattr,delattrandsetattr; prefer normal attribute access.”Proposed change
- setattr(resources.model, "generate", signaling_generate) + resources.model.generate = signaling_generateSource: Coding guidelines
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 7c55a79f-8f98-4a39-b8ac-10e2d78b7c51
📒 Files selected for processing (4)
CHANGELOG.mdplugins/elevenlabs/tests/test_elevenlabs_stt.pyplugins/huggingface/tests/test_transformers_vlm.pyplugins/sarvam/README.md
🚧 Files skipped from review as they are similar to previous changes (3)
- plugins/sarvam/README.md
- CHANGELOG.md
- plugins/elevenlabs/tests/test_elevenlabs_stt.py
Included review availability: Your plan provides up to 8 included reviews per hour; 7 remain after this review.
|
@DaemonLoki can you check the conflicts here? |
2 participants
Why
Several plugin tests and model assumptions drifted from current provider behavior, which made CI flaky or fail outright. ElevenLabs STT can emit multiple utterances per clip, HuggingFace VLM interrupt coverage needed a more reliable signal than counting streamed deltas, and Sarvam no longer offers
sarvam-30b.Changes
model.generateand assert truncated generationsarvam-30bfromsarvam.LLMand update README/tests tosarvam-105bMade with Cursor